feat: update environment configuration and enhance Header component#1585
feat: update environment configuration and enhance Header component#1585wisdom1016 wants to merge 2 commits intorecoupable:mainfrom
Conversation
- Added NEXT_PUBLIC_RECOUP_API_URL to .env.example for local development. - Refactored next.config.mjs to use remotePatterns for image domains. - Enhanced Header component to manage mobile menu visibility after mount, preventing hydration issues. Also added documentation for running the app locally in PRE_TRIAL_LOCAL.md.
|
@wisdom1016 is attempting to deploy a commit to the Recoupable Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a client-mounted guard to the header to prevent hydration mismatches, changes the non-production API base URL to localhost, and introduces a sandbox-setup flow that runs once per session after login via a new hook, helper, and provider component. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Providers
participant Auth as Auth (Privy)
participant SandboxComp as SandboxSetupOnLogin
participant Hook as useSandboxSetupOnLogin
participant API as Sandbox API (setupSandbox)
App->>Auth: initialize providers
App->>SandboxComp: render (mounted in Providers)
SandboxComp->>Hook: invoke hook on render
Hook->>Auth: read authenticated, accessToken
alt authenticated & accessToken & not done this session
Hook->>API: POST /sandbox/setup (Authorization: Bearer token)
API-->>Hook: { status: "success" | "error", ... }
Hook-->>sessionStorage: set "sandboxSetupDone" = true (on success)
else not eligible
Hook-->>Hook: no-op
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0d8c9134e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| export const NEW_API_BASE_URL = IS_PROD | ||
| ? "https://recoup-api.vercel.app" | ||
| : "https://test-recoup-api.vercel.app"; | ||
| : "http://localhost:3001"; |
There was a problem hiding this comment.
NEW_API_BASE_URL now points all non-production environments to http://localhost:3001, which causes preview/test deployments (where NEXT_PUBLIC_VERCEL_ENV !== "production") to send API calls to loopback instead of Recoup’s deployed API; any module importing this constant (e.g., hooks and server routes) will fail unless an API server is running in the same host/container. This is a regression from the previous non-prod default (https://test-recoup-api.vercel.app) and also makes the newly documented NEXT_PUBLIC_RECOUP_API_URL ineffective because this constant never reads it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/Header/Header.tsx (2)
46-53: Expose menu toggle state via ARIA attributes.The trigger button should announce open/closed state for assistive tech (
aria-expanded) and, if available, reference the controlled panel (aria-controls).♿ Suggested ARIA enhancement
<button type="button" className="md:hidden flex items-center gap-2 z-[50]" onClick={() => setIsOpenMobileMenu(!isOpenMobileMenu)} aria-label="Open menu" + aria-expanded={isOpenMobileMenu} + aria-controls="mobile-side-menu" >As per coding guidelines,
**/*.{tsx,ts,jsx,js}: Provide proper ARIA roles/states and test with screen readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Header/Header.tsx` around lines 46 - 53, The menu toggle button currently toggles state via setIsOpenMobileMenu but doesn't expose that state to assistive tech; update the button element that renders MenuIcon to include aria-expanded={isOpenMobileMenu} and add an aria-controls attribute that references the ID of the mobile panel it toggles (ensure the controlled panel component has a matching id). Use the existing isOpenMobileMenu boolean for the expanded state and keep the onClick handler (setIsOpenMobileMenu(!isOpenMobileMenu)); verify the mobile panel component/class that shows/hides the menu has the corresponding id so the aria-controls link is valid.
44-44: Use a semantic<header>for the top app bar container.Line 44 is the page header region but uses a generic
div; switching toheaderimproves semantics and accessibility tooling.♻️ Suggested semantic update
- <div className="z-[50] fixed bg-card left-0 right-0 top-0 md:hidden flex min-h-[56px] p-4 items-center justify-between w-auto"> + <header className="z-[50] fixed bg-card left-0 right-0 top-0 md:hidden flex min-h-[56px] p-4 items-center justify-between w-auto"> ... - </div> + </header>As per coding guidelines,
**/*.{tsx,ts,jsx,js}: Use semantic HTML elements appropriate to the component's role.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Header/Header.tsx` at line 44, Replace the top app bar's generic div with a semantic header element in Header (the element with className "z-[50] fixed bg-card left-0 right-0 top-0 md:hidden flex min-h-[56px] p-4 items-center justify-between w-auto"); keep all existing attributes (className, positioning, children) unchanged so layout and styling remain identical and ensure accessibility tools detect it as the page header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/consts.ts`:
- Around line 4-6: NEW_API_BASE_URL currently hardcodes "http://localhost:3001"
for non-prod which breaks staging/preview and HTTPS; change NEW_API_BASE_URL to
derive from an environment variable (e.g., NEXT_PUBLIC_API_BASE_URL or
NEXT_PUBLIC_VERCEL_URL) with a sensible fallback instead of localhost, using
IS_PROD to choose production URL; update the logic around NEW_API_BASE_URL and
any callers expecting a string so non-prod deployments can configure their real
API host and avoid mixed-content issues.
---
Nitpick comments:
In `@components/Header/Header.tsx`:
- Around line 46-53: The menu toggle button currently toggles state via
setIsOpenMobileMenu but doesn't expose that state to assistive tech; update the
button element that renders MenuIcon to include aria-expanded={isOpenMobileMenu}
and add an aria-controls attribute that references the ID of the mobile panel it
toggles (ensure the controlled panel component has a matching id). Use the
existing isOpenMobileMenu boolean for the expanded state and keep the onClick
handler (setIsOpenMobileMenu(!isOpenMobileMenu)); verify the mobile panel
component/class that shows/hides the menu has the corresponding id so the
aria-controls link is valid.
- Line 44: Replace the top app bar's generic div with a semantic header element
in Header (the element with className "z-[50] fixed bg-card left-0 right-0
top-0 md:hidden flex min-h-[56px] p-4 items-center justify-between w-auto");
keep all existing attributes (className, positioning, children) unchanged so
layout and styling remain identical and ensure accessibility tools detect it as
the page header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e141fd29-ec13-414d-a1d6-68564b0d957c
⛔ Files ignored due to path filters (3)
.env.exampleis excluded by none and included by nonedocs/PRE_TRIAL_LOCAL.mdis excluded by none and included by nonenext.config.mjsis excluded by none and included by none
📒 Files selected for processing (2)
components/Header/Header.tsxlib/consts.ts
| export const NEW_API_BASE_URL = IS_PROD | ||
| ? "https://recoup-api.vercel.app" | ||
| : "https://test-recoup-api.vercel.app"; | ||
| : "http://localhost:3001"; |
There was a problem hiding this comment.
Avoid hardcoding localhost as the non-prod API base URL.
Line 6 forces all non-prod clients to call http://localhost:3001, which breaks preview/staging usage and can fail under HTTPS due to mixed-content.
🔧 Proposed fix
export const IS_PROD = process.env.NEXT_PUBLIC_VERCEL_ENV === "production";
-export const NEW_API_BASE_URL = IS_PROD
- ? "https://recoup-api.vercel.app"
- : "http://localhost:3001";
+const DEFAULT_NON_PROD_API_BASE_URL = "https://test-recoup-api.vercel.app";
+export const NEW_API_BASE_URL =
+ process.env.NEXT_PUBLIC_RECOUP_API_URL?.trim() ||
+ (IS_PROD
+ ? "https://recoup-api.vercel.app"
+ : DEFAULT_NON_PROD_API_BASE_URL);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const NEW_API_BASE_URL = IS_PROD | |
| ? "https://recoup-api.vercel.app" | |
| : "https://test-recoup-api.vercel.app"; | |
| : "http://localhost:3001"; | |
| export const IS_PROD = process.env.NEXT_PUBLIC_VERCEL_ENV === "production"; | |
| const DEFAULT_NON_PROD_API_BASE_URL = "https://test-recoup-api.vercel.app"; | |
| export const NEW_API_BASE_URL = | |
| process.env.NEXT_PUBLIC_RECOUP_API_URL?.trim() || | |
| (IS_PROD | |
| ? "https://recoup-api.vercel.app" | |
| : DEFAULT_NON_PROD_API_BASE_URL); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/consts.ts` around lines 4 - 6, NEW_API_BASE_URL currently hardcodes
"http://localhost:3001" for non-prod which breaks staging/preview and HTTPS;
change NEW_API_BASE_URL to derive from an environment variable (e.g.,
NEXT_PUBLIC_API_BASE_URL or NEXT_PUBLIC_VERCEL_URL) with a sensible fallback
instead of localhost, using IS_PROD to choose production URL; update the logic
around NEW_API_BASE_URL and any callers expecting a string so non-prod
deployments can configure their real API host and avoid mixed-content issues.
- Introduced a new hook `useSandboxSetupOnLogin` to handle sandbox setup after user authentication. - Created `setupSandbox` function to interact with the API for sandbox initialization. - Added `SandboxSetupOnLogin` component to integrate the hook into the provider structure, ensuring the sandbox is set up seamlessly upon user login.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sandboxes/setupSandbox.ts`:
- Around line 13-27: The current setupSandbox function treats any HTTP 2xx
response as success but ignores payload-level failures signaled by
SetupSandboxResponse.status === "error"; update setupSandbox to parse the JSON
response body (await response.json()), then if the parsed payload has status ===
"error" throw an Error that includes the payload error message/details so
callers don't treat a failed setup as successful, otherwise return the parsed
payload as SetupSandboxResponse; continue to preserve the existing HTTP-level
response.ok check and include payload inspection (referencing the setupSandbox
function and the SetupSandboxResponse type).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3cb445bc-51df-4a7a-b5ea-6fce7eeba159
📒 Files selected for processing (4)
hooks/useSandboxSetupOnLogin.tsxlib/sandboxes/setupSandbox.tsproviders/Providers.tsxproviders/SandboxSetupOnLogin.tsx
| export async function setupSandbox(accessToken: string): Promise<SetupSandboxResponse> { | ||
| const response = await fetch(`${NEW_API_BASE_URL}/api/sandboxes/setup`, { | ||
| method: "POST", | ||
| headers: { | ||
| Authorization: `Bearer ${accessToken}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to setup sandbox: ${response.status} ${response.statusText}`); | ||
| } | ||
|
|
||
| return (await response.json()) as SetupSandboxResponse; | ||
| } |
There was a problem hiding this comment.
Handle API payload failures, not only HTTP failures.
SetupSandboxResponse explicitly allows status: "error", but the function resolves that payload as success today. That can mark downstream flows complete when setup actually failed.
Suggested fix
type SetupSandboxResponse =
| {
status: "success";
runId: string;
}
| {
status: "error";
error: string;
};
-export async function setupSandbox(accessToken: string): Promise<SetupSandboxResponse> {
+export async function setupSandbox(
+ accessToken: string
+): Promise<{ status: "success"; runId: string }> {
const response = await fetch(`${NEW_API_BASE_URL}/api/sandboxes/setup`, {
method: "POST",
headers: {
Authorization: `Bearer ${accessToken}`,
"Content-Type": "application/json",
},
});
+ const data = (await response.json()) as SetupSandboxResponse;
+
if (!response.ok) {
- throw new Error(`Failed to setup sandbox: ${response.status} ${response.statusText}`);
+ throw new Error(
+ data.status === "error"
+ ? data.error
+ : `Failed to setup sandbox: ${response.status} ${response.statusText}`
+ );
}
- return (await response.json()) as SetupSandboxResponse;
+ if (data.status === "error") {
+ throw new Error(data.error);
+ }
+
+ return data;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sandboxes/setupSandbox.ts` around lines 13 - 27, The current setupSandbox
function treats any HTTP 2xx response as success but ignores payload-level
failures signaled by SetupSandboxResponse.status === "error"; update
setupSandbox to parse the JSON response body (await response.json()), then if
the parsed payload has status === "error" throw an Error that includes the
payload error message/details so callers don't treat a failed setup as
successful, otherwise return the parsed payload as SetupSandboxResponse;
continue to preserve the existing HTTP-level response.ok check and include
payload inspection (referencing the setupSandbox function and the
SetupSandboxResponse type).
1 participant
Also added documentation for running the app locally in PRE_TRIAL_LOCAL.md.
Summary by CodeRabbit
New Features
Bug Fixes
Chores